fix: resolve three core architectural violations in wrapper layer#1552
fix: resolve three core architectural violations in wrapper layer#1552praisonai-triage-agent[bot] wants to merge 2 commits intomainfrom
Conversation
#1551) - Remove ToolResolver global singleton, fix cache race conditions with immutable MappingProxyType and proper locking - Fix telemetry initialization TOCTOU race with threading.Lock, prevent env var overwrites - Consolidate AgentScheduler implementations with deprecation shims pointing to canonical scheduler package - Update MCP adapters to use canonical import paths for full feature parity Follows AGENTS.md principles: no global singletons, multi-agent + async safe, DRY, sensible defaults Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR addresses concurrency and architectural issues in the wrapper layer by: (1) protecting telemetry initialization with a module-level threading lock and preventing unconditional environment variable overwrites, (2) replacing deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR addresses three architectural violations in the wrapper layer: replacing the Confidence Score: 4/5Safe to merge with minor cleanup — the two remaining issues in AsyncAgentScheduler are edge-case bugs that don't affect the happy path. All three previously flagged P1 concerns are resolved. The two new findings (missing finally in _run_schedule and unhandled task exceptions in stop()) are P2: they only manifest when the async task is cancelled externally or raises an unexpected exception, not during normal stop() usage. src/praisonai/praisonai/async_agent_scheduler.py — _run_schedule and stop() need a finally block and broader exception guard respectively. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant AsyncAgentScheduler
participant _run_schedule
participant _execute_with_retry
Caller->>AsyncAgentScheduler: start(schedule_expr)
AsyncAgentScheduler->>AsyncAgentScheduler: is_running = True
AsyncAgentScheduler->>_run_schedule: asyncio.create_task()
AsyncAgentScheduler-->>Caller: True
loop Every interval
_run_schedule->>_execute_with_retry: await
_execute_with_retry-->>_run_schedule: result / failure
_run_schedule->>_run_schedule: wait_for(_stop_event, timeout=interval)
end
Caller->>AsyncAgentScheduler: stop()
AsyncAgentScheduler->>AsyncAgentScheduler: _stop_event.set()
AsyncAgentScheduler->>_run_schedule: await wait_for(_task, timeout=10)
alt task finishes in time
_run_schedule-->>AsyncAgentScheduler: done
else timeout
AsyncAgentScheduler->>_run_schedule: cancel()
end
AsyncAgentScheduler->>AsyncAgentScheduler: is_running = False
AsyncAgentScheduler-->>Caller: True
note over _run_schedule: External cancellation skips is_running=False (no finally block)
Reviews (2): Last reviewed commit: "fix: resolve critical issues in architec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| os.getenv("LANGFUSE_PUBLIC_KEY") | ||
| or os.path.exists(os.path.expanduser("~/.praisonai/langfuse.env")) | ||
| ) | ||
| os.environ["OTEL_SDK_DISABLED"] = "false" if langfuse_configured else "true" | ||
| os.environ.setdefault("EC_TELEMETRY", "false") # respect user overrides |
There was a problem hiding this comment.
_ensure_telemetry_defaults() is dead code — never called
The PR removes the call from __getattr__ with a comment that it should be called from cli.PraisonAI.__init__ instead, but no such call was added to cli.py (confirmed by codebase search). The function has zero callers, so OTEL_SDK_DISABLED and EC_TELEMETRY defaults are never set, silently disabling the Langfuse/OTEL integration that worked before this PR.
| self.on_success = on_success | ||
| self.on_failure = on_failure | ||
|
|
||
| self._is_running = False | ||
| self._task_handle: Optional[asyncio.Task] = None | ||
| self.is_running = False | ||
| self._task: Optional[asyncio.Task] = None |
There was a problem hiding this comment.
asyncio.Event() created eagerly in __init__, risking wrong-loop binding on Python < 3.10
The old code lazy-created async primitives in _ensure_async_primitives() to bind to the caller's event loop. Moving self._stop_event = asyncio.Event() into the synchronous __init__ means on Python < 3.10 it may bind to the wrong loop, leading to RuntimeError: task attached to a different loop when await scheduler.start(…) runs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai/praisonai/mcp_server/adapters/cli_tools.py (1)
343-384:⚠️ Potential issue | 🔴 CriticalCritical: The
praisonai.schedule.*MCP tools call methods that do not exist onAgentScheduler.The handlers instantiate
AgentScheduler()with no arguments and invokelist_tasks()/add_task()/remove_task(), but the canonicalAgentSchedulerclass (atsrc/praisonai/praisonai/scheduler/agent_scheduler.py) requires__init__(agent, task, ...)and exposes onlystart(),stop(),execute_once(),get_stats(),from_yaml(), andfrom_recipe(). This causes:
AgentScheduler()→TypeError: missing 'agent' and 'task'scheduler.list_tasks()/add_task()/remove_task()→AttributeErrorBoth exceptions are caught by the outer
except Exceptionblock, silently degrading the three MCP schedule tools to permanent error responses.Correct implementations exist in
src/praisonai-agents/praisonaiagents/tools/schedule_tools.py: standaloneschedule_add(),schedule_list(), andschedule_remove()functions backed by a store (ConfigYamlScheduleStore). Import and use those instead, or create a local wrapper using the same store abstraction adapted to the MCP interface signature (task_name,cron,workflow_path).🛠️ Example — route MCP handlers to proper schedule store
`@register_tool`("praisonai.schedule.list") def schedule_list() -> str: """List scheduled tasks.""" try: - from praisonai.scheduler import AgentScheduler - scheduler = AgentScheduler() - tasks = scheduler.list_tasks() - return str(tasks) + # Use schedule store directly or import from praisonai-agents + from praisonaiagents.tools.schedule_tools import schedule_list as _schedule_list + return _schedule_list() except ImportError: return "Error: Scheduler not available" except Exception as e: return f"Error: {e}" `@register_tool`("praisonai.schedule.add") def schedule_add(task_name: str, cron: str, workflow_path: str) -> str: """Add a scheduled task.""" try: - from praisonai.scheduler import AgentScheduler - scheduler = AgentScheduler() - scheduler.add_task(task_name, cron, workflow_path) - return f"Task scheduled: {task_name}" + from praisonaiagents.scheduler.config_store import ConfigYamlScheduleStore + from praisonaiagents.scheduler.parser import parse_schedule + from praisonaiagents.scheduler.models import ScheduleJob + store = ConfigYamlScheduleStore() + sched = parse_schedule(cron) + job = ScheduleJob(name=task_name, schedule=sched, message=workflow_path) + store.add(job) + return f"Task scheduled: {task_name}" except ImportError: return "Error: Scheduler not available" except Exception as e: return f"Error: {e}" `@register_tool`("praisonai.schedule.remove") def schedule_remove(task_name: str) -> str: """Remove a scheduled task.""" try: - from praisonai.scheduler import AgentScheduler - scheduler = AgentScheduler() - scheduler.remove_task(task_name) - return f"Task removed: {task_name}" + from praisonaiagents.scheduler.config_store import ConfigYamlScheduleStore + store = ConfigYamlScheduleStore() + removed = store.remove_by_name(task_name) + return f"Task removed: {task_name}" if removed else f"Task '{task_name}' not found." except ImportError: return "Error: Scheduler not available" except Exception as e: return f"Error: {e}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/mcp_server/adapters/cli_tools.py` around lines 343 - 384, The MCP tool handlers schedule_list, schedule_add, and schedule_remove currently instantiate AgentScheduler() and call nonexistent methods, causing TypeError/AttributeError; replace their implementation to call the canonical standalone schedule helpers (schedule_list, schedule_add, schedule_remove) from praisonaiagents.tools.schedule_tools or directly use the ConfigYamlScheduleStore used there, adapting calls to the MCP signatures (task_name, cron, workflow_path) and returning the same string responses; ensure you remove the AgentScheduler() instantiation and instead import and call the correct functions or construct the ConfigYamlScheduleStore wrapper so the MCP handlers delegate to the existing schedule store APIs.
🧹 Nitpick comments (2)
src/praisonai/praisonai/tool_resolver.py (1)
56-56: Update type annotation to reflect immutable cache.
_local_tools_cacheis now reassigned toMappingProxyTypeviews in every path of_load_local_toolsandclear_cache, but the field is still annotated asDict[str, Callable].MappingProxyTypeis not aDict, so this annotation is no longer accurate (mypy/pyright will flag the assignments). The same applies to the implied return type of_load_local_tools— its docstring at line 68 already says "Immutable dict".♻️ Proposed annotation fix
-from typing import Any, Callable, Dict, List, Optional +from typing import Any, Callable, Dict, List, Mapping, Optional from types import MappingProxyType @@ - self._tools_py_path = tools_py_path or "tools.py" - self._local_tools_cache: Dict[str, Callable] = {} - self._local_tools_loaded: bool = False + self._tools_py_path = tools_py_path or "tools.py" + self._local_tools_cache: Mapping[str, Callable] = MappingProxyType({}) + self._local_tools_loaded: bool = False @@ - def _load_local_tools(self) -> Dict[str, Callable]: + def _load_local_tools(self) -> Mapping[str, Callable]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/tool_resolver.py` at line 56, The field _local_tools_cache is annotated as Dict[str, Callable] but is assigned MappingProxyType (immutable mappings) in _load_local_tools and clear_cache; update the annotation to use typing.Mapping (e.g., Mapping[str, Callable]) to reflect immutability and avoid type checker errors, and adjust any function return/type hints for _load_local_tools accordingly (ensure imports include Mapping from typing and reference _local_tools_cache, _load_local_tools, and clear_cache in the edits).src/praisonai/praisonai/async_agent_scheduler.py (1)
184-198: Suggestion: make the 10s graceful-stop timeout configurable / proportional.If
_execute_with_retryis currently mid-call to a long-running agent (LLM round-trip + retry/backoff), the 10swait_forwill fire and_task.cancel()will hard-interrupt that work — including any in-flight network/IO from the agent. That’s a defensible default, but it meansstop()can lose work silently.Consider:
- Accepting
timeout: float = 10.0onstop(...)so callers running long agents can override.- Logging the current execution attempt (or remaining retries) when the cancel path is taken so operators understand what was interrupted.
🛠️ Configurable timeout
- async def stop(self) -> bool: + async def stop(self, timeout: float = 10.0) -> bool: @@ - if self._task: - try: - await asyncio.wait_for(self._task, timeout=10) - except asyncio.TimeoutError: - logger.warning("Scheduler task didn't stop gracefully, cancelling") + if self._task: + try: + await asyncio.wait_for(self._task, timeout=timeout) + except asyncio.TimeoutError: + logger.warning( + "Scheduler task didn't stop within %.1fs, cancelling (current " + "execution will be interrupted)", timeout, + ) self._task.cancel()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/async_agent_scheduler.py` around lines 184 - 198, Change stop() to accept a configurable timeout: add a parameter like timeout: float = 10.0 to stop and use that value in asyncio.wait_for(self._task, timeout=timeout). When cancelling the task (self._task.cancel()), log contextual execution state from the running worker (e.g., include identifiers or retry state from _execute_with_retry such as current attempt, remaining_retries, or current_agent_call info) via logger.warning so operators can see what was interrupted; ensure you reference self._task and _execute_with_retry in the log. Keep the existing cancel/await CancelledError handling and preserve returning True.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/__init__.py`:
- Around line 42-67: The _ensure_telemetry_defaults() function is never invoked,
so its Langfuse-aware OTEL logic is dead; either call it from PraisonAI.__init__
or restore the prior __getattr__ call and remove the unconditional
os.environ.setdefault("OTEL_SDK_DISABLED", "true") in cli/main.py. Update
PraisonAI.__init__ to call _ensure_telemetry_defaults() early (and remove or
change the unconditional setdefault in main()) OR reintroduce a call to
_ensure_telemetry_defaults() inside __getattr__ so the function runs lazily;
ensure you keep the idempotent behavior and the
_telemetry_lock/_telemetry_initialized checks intact.
In `@src/praisonai/praisonai/agent_scheduler.py`:
- Around line 15-18: The shim imports a non-existent symbol
AgentExecutorInterface causing ImportError; update the import/export to use the
real ExecutorInterface and expose it under the old name: import
ExecutorInterface (from praisonai.scheduler.agent_scheduler or
praisonai.scheduler.base, whichever actually exports it) alongside
AgentScheduler, PraisonAgentExecutor, create_agent_scheduler, then add an alias
AgentExecutorInterface = ExecutorInterface so the shim continues to provide the
legacy name without breaking imports.
In `@src/praisonai/praisonai/async_agent_scheduler.py`:
- Around line 9-17: The deprecation message incorrectly directs users to import
AsyncAgentScheduler from praisonai.scheduler before that symbol is exposed;
update the warnings.warn invocation in this module to be truthful until the
migration is done by replacing DeprecationWarning with PendingDeprecationWarning
(or remove the warning entirely) and change the message to instruct users to
continue importing from praisonai.async_agent_scheduler for now (keeping the
existing re-export/imports of ScheduleParser, backoff_delay, safe_call intact);
ensure you update the warnings.warn call (and its message text) for
AsyncAgentScheduler so it no longer promises a non-existent public API.
---
Outside diff comments:
In `@src/praisonai/praisonai/mcp_server/adapters/cli_tools.py`:
- Around line 343-384: The MCP tool handlers schedule_list, schedule_add, and
schedule_remove currently instantiate AgentScheduler() and call nonexistent
methods, causing TypeError/AttributeError; replace their implementation to call
the canonical standalone schedule helpers (schedule_list, schedule_add,
schedule_remove) from praisonaiagents.tools.schedule_tools or directly use the
ConfigYamlScheduleStore used there, adapting calls to the MCP signatures
(task_name, cron, workflow_path) and returning the same string responses; ensure
you remove the AgentScheduler() instantiation and instead import and call the
correct functions or construct the ConfigYamlScheduleStore wrapper so the MCP
handlers delegate to the existing schedule store APIs.
---
Nitpick comments:
In `@src/praisonai/praisonai/async_agent_scheduler.py`:
- Around line 184-198: Change stop() to accept a configurable timeout: add a
parameter like timeout: float = 10.0 to stop and use that value in
asyncio.wait_for(self._task, timeout=timeout). When cancelling the task
(self._task.cancel()), log contextual execution state from the running worker
(e.g., include identifiers or retry state from _execute_with_retry such as
current attempt, remaining_retries, or current_agent_call info) via
logger.warning so operators can see what was interrupted; ensure you reference
self._task and _execute_with_retry in the log. Keep the existing cancel/await
CancelledError handling and preserve returning True.
In `@src/praisonai/praisonai/tool_resolver.py`:
- Line 56: The field _local_tools_cache is annotated as Dict[str, Callable] but
is assigned MappingProxyType (immutable mappings) in _load_local_tools and
clear_cache; update the annotation to use typing.Mapping (e.g., Mapping[str,
Callable]) to reflect immutability and avoid type checker errors, and adjust any
function return/type hints for _load_local_tools accordingly (ensure imports
include Mapping from typing and reference _local_tools_cache, _load_local_tools,
and clear_cache in the edits).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce5a3346-7336-4a29-ade6-529163dfd7e1
📒 Files selected for processing (5)
src/praisonai/praisonai/__init__.pysrc/praisonai/praisonai/agent_scheduler.pysrc/praisonai/praisonai/async_agent_scheduler.pysrc/praisonai/praisonai/mcp_server/adapters/cli_tools.pysrc/praisonai/praisonai/tool_resolver.py
| def _ensure_telemetry_defaults() -> None: | ||
| """Apply telemetry env defaults exactly once, on first observability use.""" | ||
| global _telemetry_initialized | ||
| if _telemetry_initialized: | ||
| if _telemetry_initialized: # fast path, OK without lock | ||
| return | ||
| import os | ||
| langfuse_configured = bool( | ||
| os.getenv("LANGFUSE_PUBLIC_KEY") | ||
| or os.path.exists(os.path.expanduser("~/.praisonai/langfuse.env")) | ||
| ) | ||
| if langfuse_configured: | ||
| # Explicitly enable OTEL for Langfuse integration | ||
| os.environ["OTEL_SDK_DISABLED"] = "false" | ||
| else: | ||
| os.environ.setdefault("OTEL_SDK_DISABLED", "true") | ||
| os.environ.setdefault("EC_TELEMETRY", "false") # respect user overrides | ||
| _telemetry_initialized = True | ||
| with _telemetry_lock: | ||
| if _telemetry_initialized: | ||
| return | ||
| import os | ||
| # Respect any value the user already set | ||
| if "OTEL_SDK_DISABLED" not in os.environ: | ||
| langfuse_configured = bool( | ||
| os.getenv("LANGFUSE_PUBLIC_KEY") | ||
| or os.path.exists(os.path.expanduser("~/.praisonai/langfuse.env")) | ||
| ) | ||
| os.environ["OTEL_SDK_DISABLED"] = "false" if langfuse_configured else "true" | ||
| os.environ.setdefault("EC_TELEMETRY", "false") # respect user overrides | ||
| _telemetry_initialized = True | ||
|
|
||
|
|
||
| # Lazy loading for heavy imports | ||
| def __getattr__(name): | ||
| """Lazy load heavy modules to improve import time.""" | ||
| # Ensure telemetry defaults before any lazy import that may touch OTEL. | ||
| _ensure_telemetry_defaults() | ||
| # Note: Telemetry initialization moved out of lazy hook to avoid side effects | ||
| # It should be called explicitly from cli.PraisonAI.__init__ instead | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every reference to _ensure_telemetry_defaults across the repo
rg -nP -C2 '\b_ensure_telemetry_defaults\b'
# And specifically inside the cli package, where the comment says it should live
fd -t f -e py . src/praisonai/praisonai/cli 2>/dev/null | xargs rg -nP -C2 '\b_ensure_telemetry_defaults\b' || true
# Also verify the unconditional OTEL setdefault in main.py is still present and unconditional
rg -nP -C3 'OTEL_SDK_DISABLED' src/praisonai/praisonaiRepository: MervinPraison/PraisonAI
Length of output: 2576
Telemetry init function is dead code — never invoked anywhere in the codebase.
_ensure_telemetry_defaults() is defined but never called. The __getattr__ invocation was removed and the comment at lines 65–66 directs callers to invoke it from cli.PraisonAI.__init__, but no such call exists. Instead, cli/main.py:336 unconditionally runs os.environ.setdefault("OTEL_SDK_DISABLED", "true"), which bypasses the Langfuse-aware logic in lines 53–57 (that checks LANGFUSE_PUBLIC_KEY and ~/.praisonai/langfuse.env).
Result: the function's smart telemetry detection never runs in normal flows, and the telemetry environment variable defaults are not applied to non-CLI consumers either.
Either:
- Call
_ensure_telemetry_defaults()fromPraisonAI.__init__(and remove or replace the unconditionalsetdefaultinmain()so it respects the Langfuse check), or - Restore the call in
__getattr__(it is idempotent and lock-protected, so the original concern is mitigated).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/__init__.py` around lines 42 - 67, The
_ensure_telemetry_defaults() function is never invoked, so its Langfuse-aware
OTEL logic is dead; either call it from PraisonAI.__init__ or restore the prior
__getattr__ call and remove the unconditional
os.environ.setdefault("OTEL_SDK_DISABLED", "true") in cli/main.py. Update
PraisonAI.__init__ to call _ensure_telemetry_defaults() early (and remove or
change the unconditional setdefault in main()) OR reintroduce a call to
_ensure_telemetry_defaults() inside __getattr__ so the function runs lazily;
ensure you keep the idempotent behavior and the
_telemetry_lock/_telemetry_initialized checks intact.
| from praisonai.scheduler.agent_scheduler import ( # noqa: F401 | ||
| AgentScheduler, PraisonAgentExecutor, AgentExecutorInterface, | ||
| create_agent_scheduler | ||
| ) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether AgentExecutorInterface exists anywhere under the scheduler package,
# and whether ExecutorInterface is the actual canonical symbol.
fd -t f -e py . | xargs rg -nP '\bAgentExecutorInterface\b' || true
echo '---'
rg -nP '^\s*class\s+(Executor|AgentExecutor)Interface\b' --type=py
echo '---'
# Show what the canonical scheduler package re-exports.
fd -a 'scheduler/__init__.py' --type=f | xargs -I{} sh -c 'echo "==> {}"; cat "{}"'Repository: MervinPraison/PraisonAI
Length of output: 534
🏁 Script executed:
# Check what agent_scheduler.py actually exports
cat -n src/praisonai/praisonai/scheduler/agent_scheduler.py | head -100
echo "---"
# Check the base.py to understand ExecutorInterface
cat -n src/praisonai/praisonai/scheduler/base.py | sed -n '70,130p'
echo "---"
# Check scheduler/__init__.py if it exists
find src/praisonai/praisonai/scheduler -name '__init__.py' -exec cat -n {} \;Repository: MervinPraison/PraisonAI
Length of output: 7902
Critical: AgentExecutorInterface is not exported by praisonai.scheduler.agent_scheduler — shim will raise ImportError on every import.
The canonical module exports ExecutorInterface (defined in praisonai.scheduler.base line 74) and PraisonAgentExecutor, but there is no AgentExecutorInterface symbol anywhere in the codebase. As written, importing this shim — i.e. import praisonai.agent_scheduler or any from praisonai.agent_scheduler import … — will fail with ImportError: cannot import name 'AgentExecutorInterface', which completely breaks the backward-compatibility contract this file is meant to preserve.
Alias ExecutorInterface as AgentExecutorInterface in the deprecation shim.
🛠️ Suggested fix
-from praisonai.scheduler.agent_scheduler import ( # noqa: F401
- AgentScheduler, PraisonAgentExecutor, AgentExecutorInterface,
- create_agent_scheduler
-)
+from praisonai.scheduler.agent_scheduler import ( # noqa: F401
+ AgentScheduler,
+ PraisonAgentExecutor,
+ create_agent_scheduler,
+)
+# Preserve the legacy public name as an alias of the canonical interface.
+from praisonai.scheduler.base import ExecutorInterface as AgentExecutorInterface # noqa: F401🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/agent_scheduler.py` around lines 15 - 18, The shim
imports a non-existent symbol AgentExecutorInterface causing ImportError; update
the import/export to use the real ExecutorInterface and expose it under the old
name: import ExecutorInterface (from praisonai.scheduler.agent_scheduler or
praisonai.scheduler.base, whichever actually exports it) alongside
AgentScheduler, PraisonAgentExecutor, create_agent_scheduler, then add an alias
AgentExecutorInterface = ExecutorInterface so the shim continues to provide the
legacy name without breaking imports.
| warnings.warn( | ||
| "praisonai.async_agent_scheduler is deprecated; " | ||
| "use 'from praisonai.scheduler import AsyncAgentScheduler' instead.", | ||
| DeprecationWarning, stacklevel=2, | ||
| ) | ||
|
|
||
| # TODO: Once AsyncAgentScheduler is moved to scheduler package, import from there | ||
| # For now, re-export the existing implementation to avoid breaking changes | ||
| from .scheduler.shared import ScheduleParser, backoff_delay, safe_call |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether AsyncAgentScheduler is reachable from praisonai.scheduler today.
fd -a 'scheduler/__init__.py' --type=f | xargs -I{} sh -c 'echo "==> {}"; cat "{}"'
echo '---'
rg -nP '\bclass\s+AsyncAgentScheduler\b' --type=py
echo '---'
# Find first-party callers still using the legacy path, so we know who will see this warning.
rg -nP '\bfrom\s+praisonai\.async_agent_scheduler\b|\bimport\s+praisonai\.async_agent_scheduler\b' --type=pyRepository: MervinPraison/PraisonAI
Length of output: 154
🏁 Script executed:
#!/bin/bash
# 1. List all files under scheduler directories
echo "=== Scheduler directory structure ==="
find src/praisonai -type d -name "scheduler" -o -type d -name "*scheduler*" 2>/dev/null | head -20
echo ""
# 2. Find and show praisonai/scheduler/__init__.py if it exists
echo "=== praisonai/scheduler/__init__.py (if exists) ==="
cat src/praisonai/praisonai/scheduler/__init__.py 2>/dev/null || echo "[file not found]"
echo ""
# 3. Find all AsyncAgentScheduler usages (imports and instantiations)
echo "=== AsyncAgentScheduler imports across codebase ==="
rg -n 'AsyncAgentScheduler' --type=py -B1 -A1 | head -50Repository: MervinPraison/PraisonAI
Length of output: 3990
🏁 Script executed:
#!/bin/bash
# Search for imports of AsyncAgentScheduler from outside the file where it's defined
echo "=== External imports of AsyncAgentScheduler ==="
rg -n 'from.*async_agent_scheduler.*import|import.*async_agent_scheduler' --type=py | grep -v 'src/praisonai/praisonai/async_agent_scheduler.py'
echo ""
# Also check for any usage patterns (instantiation, type hints, etc.)
echo "=== All files mentioning AsyncAgentScheduler ==="
rg -l 'AsyncAgentScheduler' --type=py | sort
echo ""
# Check if there's a __init__.py at praisonai package level that might re-export it
echo "=== praisonai/__init__.py content (relevant parts) ==="
grep -n 'AsyncAgentScheduler\|async_agent_scheduler' src/praisonai/praisonai/__init__.py 2>/dev/null || echo "[not found in __init__.py]"Repository: MervinPraison/PraisonAI
Length of output: 297
Major: the deprecation message points users to a module that does not yet expose AsyncAgentScheduler.
The TODO on lines 15–16 explicitly states that AsyncAgentScheduler has not been moved to praisonai.scheduler yet, but the warning on lines 9–13 instructs users to do from praisonai.scheduler import AsyncAgentScheduler immediately. Attempting this import will raise AttributeError because praisonai.scheduler.__init__.py does not expose AsyncAgentScheduler in its public API. This contradicts the deprecation cycle principle: during a transition period, the deprecation message must continue to point users toward a working import path.
Pick one of:
- Move the implementation into
praisonai/scheduler/async_agent_scheduler.pynow and reduce this file to a re-export shim (preferred — matches the sync counterpartagent_scheduler.py). - Rewrite the warning to reflect current reality: drop the DeprecationWarning until the migration is complete, or use
PendingDeprecationWarningand instruct users to continue importing frompraisonai.async_agent_schedulerfor now.
🛠️ Option 2 — make the warning truthful until the move happens
warnings.warn(
- "praisonai.async_agent_scheduler is deprecated; "
- "use 'from praisonai.scheduler import AsyncAgentScheduler' instead.",
- DeprecationWarning, stacklevel=2,
+ "praisonai.async_agent_scheduler is pending deprecation; it will be moved to "
+ "praisonai.scheduler.async_agent_scheduler in a future release. Continue "
+ "importing from praisonai.async_agent_scheduler until then.",
+ PendingDeprecationWarning,
+ stacklevel=2,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/async_agent_scheduler.py` around lines 9 - 17, The
deprecation message incorrectly directs users to import AsyncAgentScheduler from
praisonai.scheduler before that symbol is exposed; update the warnings.warn
invocation in this module to be truthful until the migration is done by
replacing DeprecationWarning with PendingDeprecationWarning (or remove the
warning entirely) and change the message to instruct users to continue importing
from praisonai.async_agent_scheduler for now (keeping the existing
re-export/imports of ScheduleParser, backoff_delay, safe_call intact); ensure
you update the warnings.warn call (and its message text) for AsyncAgentScheduler
so it no longer promises a non-existent public API.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review - PR #1552 ✅ APPROVEDTodo List:
Architecture Review Summary✅ PASSES all AGENTS.md criteria:
Issues Fixed (All Critical Issues Resolved)✅ P1 Critical -
✅ P1 Critical - Telemetry defaults never applied:
✅ P2 - MCP schedule tools broken:
✅ Type annotations:
✅ Misleading deprecation warning:
Files Modified & Pushed (Commit 72393e3)
Final Verdict: APPROVED ✅The PR successfully addresses all three core architectural violations as intended:
All critical regressions identified by multiple reviewers have been resolved. The changes follow AGENTS.md architecture guidelines and maintain full backward compatibility while fixing the core issues. Recommendation: Ready to merge after CI passes. |
- Fix AgentExecutorInterface import error by aliasing ExecutorInterface - Fix dead telemetry code by calling _ensure_telemetry_defaults in PraisonAI.__init__ - Fix MCP schedule tools to use proper SDK schedule_tools instead of broken AgentScheduler calls - Fix type annotations in tool_resolver.py to use Mapping for immutable cache - Fix misleading deprecation warning in async_agent_scheduler.py These fixes resolve the 2 P1 critical regressions and 1 P2 issue identified by multiple reviewers (Greptile, CodeRabbit, Copilot). Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #1551
This PR addresses the three architectural violations identified in the wrapper layer that violate core principles around global singletons, thread safety, and DRY.
Changes Made
1. ToolResolver Global Singleton + Cache Race Conditions
2. Telemetry Initialization TOCTOU Race
3. Duplicate AgentScheduler Implementations
Testing
Generated with Claude Code
Summary by CodeRabbit
Deprecations
AgentSchedulerandAsyncAgentSchedulermodules now emit deprecation warnings; migrate to thepraisonai.schedulermodule.Breaking Changes
resolve_tool,resolve_tools,list_available_tools,has_tool,validate_yaml_tools) now accept an optional resolver parameter.Improvements